Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow --machine/--allow-embedding to be passed to devtools_tool serve #6670

Merged
merged 2 commits into from
Nov 8, 2023
Merged

Allow --machine/--allow-embedding to be passed to devtools_tool serve #6670

merged 2 commits into from
Nov 8, 2023

Conversation

DanTup
Copy link
Contributor

@DanTup DanTup commented Nov 7, 2023

The arg parser allows additional arguments, but not additional "options", so we need to add this explicitly* to allow --machine to work. The code already passes the extra args through to the server correctly, it just wasn't allowing this option to be provided on the command line.

* Or use the AllowAnythingParser, but looks like that would affect the existing options.

@DanTup DanTup marked this pull request as draft November 7, 2023 15:17
@DanTup
Copy link
Contributor Author

DanTup commented Nov 7, 2023

(converted to draft... there are other flags we pass in VS Code like --try-ports we also need to support)

@DanTup
Copy link
Contributor Author

DanTup commented Nov 7, 2023

Actually, I'm ditching --try-ports from VS Code since we're passing 10 which is the default value, but we did need allow-embedding.

@DanTup DanTup marked this pull request as ready for review November 7, 2023 15:24
@DanTup DanTup changed the title Allow --machine to be passed to devtools_tool serve Allow --machine/--allow-embedding to be passed to devtools_tool serve Nov 7, 2023
@DanTup
Copy link
Contributor Author

DanTup commented Nov 8, 2023

(this is ready for review now and hopefully the bots will be green 🤞)

kenzieschmoll
kenzieschmoll approved these changes Nov 8, 2023
View reviewed changes
Copy link
Member

@kenzieschmoll kenzieschmoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that these weren't passed through automatically since we pass any unhandled arguments along to the dds command. but lgtm

@DanTup
Copy link
Contributor Author

DanTup commented Nov 8, 2023

I'm surprised that these weren't passed through automatically since we pass any unhandled arguments along to the dds command

I was too.. seems to be a difference between arguments and the named options. There's an AllowAnythingParser that would allow these, but I think we'd then have to also read the other options ourselves, so since we only need these two I figured this was better. We can always revisit if we want to pass more in future.

@DanTup DanTup merged commit 7cdf89a into flutter:master Nov 8, 2023
19 checks passed
@DanTup DanTup deleted the allow-serve-machine branch November 8, 2023 22:03
@DanTup DanTup mentioned this pull request Feb 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kenzieschmoll kenzieschmoll kenzieschmoll approved these changes

@CoderDake CoderDake Awaiting requested review from CoderDake

Assignees
No one assigned
Labels
release-notes-not-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DanTup @kenzieschmoll